Skip to content

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented Jun 28, 2025

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 28, 2025
@petrochenkov
Copy link
Contributor

I don't understand why this works and how it fixes the issue.
The LookAheadMacroDefinition are inserted for all macro definitions, regardless of whether it's macro or macro_rules, and nothing is done for use items.
The reversed rib walk in fn apply_pattern_bindings can also encounter arbitrary ribs before encountering a LookAheadMacroDefinition.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 30, 2025
@petrochenkov
Copy link
Contributor

Also "shallow" -> "shadow" in the PR/commit messages and file names.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Jul 1, 2025

The reversed rib walk in fn apply_pattern_bindings can also encounter arbitrary ribs before encountering a LookAheadMacroDefinition.

There may be a bug if LookAheadMacroDefinition fails to apply correctly.

I don't understand why this works and how it fixes the issue.

LookAheadMacroDefinition stores the macro expansion result and serves as a fallback in resolve_ident_in_lexical_scope, ensuring it takes the correct resolution path:

// The ident resolves to a type parameter or local variable.
return Some(LexicalScopeBinding::Res(self.validate_res_from_ribs(
i,
rib_ident,
*res,
finalize.map(|finalize| finalize.path_span),
*original_rib_ident_def,
ribs,
)));

@bvanjoi bvanjoi changed the title fresh binding should shallow the def after expand fresh binding should shadow the def in expand Jul 1, 2025
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Jul 1, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 1, 2025
@petrochenkov
Copy link
Contributor

@bvanjoi
Are you sure this cannot successfully resolve some names that should not be resolved?
(I started reviewing yesterday, but it was late so I'll finish today or on Monday.)

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Jul 12, 2025

Are you sure this cannot successfully resolve some names that should not be resolved?

I need to test this with additional cases across different rib contexts. @rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 12, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 26, 2025
@rust-log-analyzer

This comment has been minimized.

@petrochenkov petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 25, 2025
@petrochenkov
Copy link
Contributor

Should we introduce this change as a lint instead?

We'll definitely need a crater run and possibly a lint in the end.

But at this point we probably rather need some more or less formal model of the rules that we want to achieve.
At both call site (including code coming from the macro itself) and at def site local variables and items can interleave, so it needs to be clarified what are the priorities.
I.e. what is the search order here.

#![allow(unused)]

fn f() {
    let a = 0;
    {
        fn a() {}
        {
            let a = 0;
            {
                fn a() {}
                
                #[macro_export]
                macro_rules! mac {
                    () => {
                        let a = 0;
                        {
                            fn a() {}
                            {
                                let a = 0;
                                {
                                    a; // use
                                }
                            }
                        }
                    };
                }
            }
        }
    }
}

fn main() {
    let a = 0;
    {
        fn a() {}
        {
            let a = 0;
            {
                mac!();
            }
        }
    }
}

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 25, 2025
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Aug 30, 2025

But at this point we probably rather need some more or less formal model of the rules that we want to achieve.

Have there been any mathematical models proposed, such as proof trees in type theory, to formally describe macro expansion? I've never encountered.😂

@petrochenkov
Copy link
Contributor

@bvanjoi
https://www-old.cs.utah.edu/plt/scope-sets
That's what macros 2.0 hygiene is inspired by as well.

@rustbot
Copy link
Collaborator

rustbot commented Aug 31, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Aug 31, 2025

what is the search order here.

I will try to answer this after reading this book.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 31, 2025
@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

Let's benchmark this to understand the costs.
@bors2 try @rust-timer queue

Another high level alternative to this is to just report errors for everything that cannot be resolved correctly using some single pass algorithm similar to what was used before.
It may just be too costly to implement proper def-site hygiene for local variables.

@rust-timer

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request Sep 2, 2025
fresh binding should shadow the def in expand
@rust-bors

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 2, 2025
@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 2, 2025
@rust-bors
Copy link

rust-bors bot commented Sep 2, 2025

☀️ Try build successful (CI)
Build commit: 47ce876 (47ce876df58648fb78e86c74948d24c6476569a4, parent: f6df223ea8c0017e64ce19c99afa32c0c629142c)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (47ce876): comparison URL.

Overall result: ❌ regressions - BENCHMARK(S) FAILED

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

❗ ❗ ❗ ❗ ❗
Warning ⚠️: The following benchmark(s) failed to build:

  • runtime:css

❗ ❗ ❗ ❗ ❗

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
15.4% [0.2%, 534.1%] 222
Regressions ❌
(secondary)
15.8% [0.0%, 543.8%] 159
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.2% [-0.2%, -0.2%] 12
All ❌✅ (primary) 15.4% [0.2%, 534.1%] 222

Max RSS (memory usage)

Results (primary -0.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
1.7% [1.7%, 1.7%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.6% [-1.6%, -1.5%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.5% [-1.6%, 1.7%] 3

Cycles

Results (primary 27.0%, secondary 61.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
27.0% [1.4%, 266.7%] 62
Regressions ❌
(secondary)
61.7% [2.2%, 260.9%] 24
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 27.0% [1.4%, 266.7%] 62

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 467.107s -> 468.456s (0.29%)
Artifact size: 388.34 MiB -> 388.37 MiB (0.01%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 2, 2025
@petrochenkov petrochenkov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 3, 2025
@petrochenkov
Copy link
Contributor

Yeah, the current approach (or at least the current implementation) is not going to fly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hygiene of used macro item is weird.
6 participants